-
-
Notifications
You must be signed in to change notification settings - Fork 75
Chore: Refactor the codebase (fixes #220) #261
Conversation
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this looks great! I like how you broke util functions into its own files. We could probably start to do that with the node converts as well at some point.
Merging this in will cause all current PR to need rebase. Not sure what order will work best.
.travis.yml
Outdated
@@ -1,7 +1,6 @@ | |||
language: node_js | |||
sudo: false | |||
node_js: | |||
- "0.12" | |||
- 4 | |||
- 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add Node v7 as well?
lib/node-utils.js
Outdated
/** | ||
* Expose the enum of possible TSNode `kind`s. | ||
*/ | ||
exports.SyntaxKind = SyntaxKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe we should move the exports to the bottom of the file and do it all in one object. That way it might be easier to know what a file exports. If this was typescript we could just export the function or variables but I don't care too much.
LGTM |
@soda0289 Yes fair point about the readability of exports, thanks! I have addressed that, and dialed up the linting strictness to fully match eslint/eslint (caught a lot of additional formatting points in test and Makefiles etc). I updated the travis to run on 4, 5, 6 and 7, again to match eslint/eslint. With regards to merging, I would love to get this in ASAP, and then continue working on more fixes. Are you happy to close out your old PRs and start branching off from this work? |
Yea I can do that. |
Thanks, sorry that this is awkward one - long overdue! I think I will get Pajn's breaking change one in first, then rebase this and merge it. Then we could do an actual release I think, because all the breaking changes will be in. |
Could we merge this into: I would like to get the comment scanner fixed before we release. Could you review that PR? |
LGTM |
Both of those are in now, thanks! |
Lets merge this then. I'll rebase the comment scanner code off of this change. |
🎉 |
This is major refactoring of the structure of the core codebase.
There are no changes to any existing test cases.
Summary of changes:
>=4
to align witheslint/eslint
object-assign
dependency, and replace its usage with nativeObject.assign
features.js
andtoken-translator.js
- they were never usedeslint/eslint
ast-node-types.js
to include all known convertedESTreeNode
typeserrorOnUnknownASTType
parser option to throw if a generatedESTreeNode
type is not found inast-node-types.js
(this is disabled by default, but enabled in tests)node-utils.js
, to make the codebase much easier to reason about, especially w.r.t. which helpers mutate the current node, and which are pure functionsts.*
helper functions into abstractions withinnode-utils.js
so that it is clear what our dependencies on TypeScript actually arefindFirstMatchingChild
andfindFirstMatchingAncestor
which take inpredicate
functions, and are built upon by other utility functions. These also allow me to remove somets.*
dependencies from the codebaseconvert()
function into its own file,convert.js
, and increase what is explicitly passed into it rather than just closing over all outer scopes. Again, this greatly helps with clarity and focused responsibilityconvert.js
, taking advantage oflet
andconst
for variable declarations. No more battling to come up with unique variable names within a 1000+ line function!